Use `ObjectStoreLocationProvider` by default #1509

smaheshwar-pltr · 2025-01-11T15:54:48Z

Following the discussion in #1452 (comment), this PR proposes diverging from the historical, Java-side preference of disabling object storage location provision by default, and proposes setting the OBJECT_STORE_ENABLED_DEFAULT table property default to True in PyIceberg.

smaheshwar-pltr · 2025-01-11T15:56:20Z

tests/integration/test_writes/test_partitioned_writes.py

@@ -297,8 +297,8 @@ def test_object_storage_location_provider_excludes_partition_path(
    tbl = _create_table(
        session_catalog=session_catalog,
        identifier=f"default.arrow_table_v{format_version}_with_null_partitioned_on_col_{part_col}",
-        # write.object-storage.partitioned-paths defaults to True
-        properties={"format-version": str(format_version), TableProperties.OBJECT_STORE_ENABLED: True},
+        # Both write.object-storage.enabled and write.object-storage.partitioned-paths default to True


Just changing this one integration test to test that it's now the default; I've kept test_writes::test_object_storage_data_files unchanged.

wydt about adding assert statements as well? comments can become stale over time

smaheshwar-pltr · 2025-01-11T16:07:08Z

Want to strongly highlight for folks reviewing (@kevinjqliu + @Fokko?) that I've not changed WRITE_OBJECT_STORE_PARTITIONED_PATHS_DEFAULT to False even though it's a similar change that intends to improves object store performance. I'm very open to discuss/change this (I'll update this PR name + description or put up a new one if so).

So do we want to do this? My thinking was: after reading through apache/iceberg#11112 it seems there was a strong case for supporting partition values in paths. When I thought about that, I realised that users might be relying on storage layout and files constituting partitions lying in the same directory - maybe just so that they can inspect storage to see how their files are actually laid out at a given point in time, especially in the context of partition evolution.

On the other hand, maybe we should make this change to discourage reliance on file storage layout. (It would also bring object storage performance improvements by default which is a plus)

smaheshwar-pltr · 2025-01-11T16:10:11Z

The main downside of this PR is that file paths and their directory structures are more obscured by default with a bunch of hash directories inserted in the middle. But IMHO this is a reasonable tradeoff (and doesn't feel as dramatic as excluding partition information from file paths entirely). It won't change existing data file paths and they'll still work fine.

kevinjqliu

Thanks for the PR!

In terms of the default value for WRITE_OBJECT_STORE_PARTITIONED_PATHS, here are the four different scenarios. (source)

WRITE_OBJECT_STORE_PARTITIONED_PATHS=True for Partitioned Table: f"{partition_key.to_path()}/{data_file_name}"
WRITE_OBJECT_STORE_PARTITIONED_PATHS=True for NonPartitioned Table: f"{prefix}/{hashed_path}/{data_file_name}"
WRITE_OBJECT_STORE_PARTITIONED_PATHS=False for Partitioned Table: f"{prefix}/{hashed_path}-{data_file_name}"
WRITE_OBJECT_STORE_PARTITIONED_PATHS=False for NonPartitioned Table: f"{prefix}/{hashed_path}-{data_file_name}"

Option 1 aligns with the current SimpleLocationProvider implementation for Partitioned Table. I think there is some benefits of having the partition_key.to_path() as the default since it's easier to reason about than a bunch of hashes. Furthermore, I look at ObjectStoreLocationProvider as S3/Object Store optimization and should only be turned on in specific scenarios.

My preference would be to keep WRITE_OBJECT_STORE_PARTITIONED_PATHS=True to satisfy option 1.

kevinjqliu · 2025-01-11T18:55:31Z

pyiceberg/table/__init__.py

@@ -190,7 +190,7 @@ class TableProperties:
    WRITE_PY_LOCATION_PROVIDER_IMPL = "write.py-location-provider.impl"

    OBJECT_STORE_ENABLED = "write.object-storage.enabled"
-    OBJECT_STORE_ENABLED_DEFAULT = False
+    OBJECT_STORE_ENABLED_DEFAULT = True  # Differs from the Java implementation


nit: instead of the comment here, let's add it to the docs

Sounds good! I've remove for now.

Then we can add all the new location-relevant write options introduced in #1452 when we address #1510 together with the other docs?

kevinjqliu · 2025-01-11T18:56:13Z

tests/integration/test_writes/test_partitioned_writes.py

@@ -297,8 +297,8 @@ def test_object_storage_location_provider_excludes_partition_path(
    tbl = _create_table(
        session_catalog=session_catalog,
        identifier=f"default.arrow_table_v{format_version}_with_null_partitioned_on_col_{part_col}",
-        # write.object-storage.partitioned-paths defaults to True
-        properties={"format-version": str(format_version), TableProperties.OBJECT_STORE_ENABLED: True},
+        # Both write.object-storage.enabled and write.object-storage.partitioned-paths default to True


wydt about adding assert statements as well? comments can become stale over time

smaheshwar-pltr · 2025-01-11T19:59:16Z

In terms of the default value for WRITE_OBJECT_STORE_PARTITIONED_PATHS, here are the four different scenarios. (source)

WRITE_OBJECT_STORE_PARTITIONED_PATHS=True for Partitioned Table: f"{partition_key.to_path()}/{data_file_name}"

Small correction @kevinjqliu - the function recurses on this line with this value being the new file name - it doesn't return it. So, hashes are still prepended to the file name after the prefix, and it's not the same as when SimpleLocationProvider is used.

To clarify, this isn't a bug. The ObjectStoreLocationProvider should include entropies to reduce prefix collision - that shouldn't change just because data is partitioned. I realise it was written confusingly, sorry about that. I matched the Java implementation here, and FWIW I suspect this confusing "recursive" (in Java, it's overloading not recursion) implementation is what led to what I've described here as a potential bug (/unintended result) on their part.

I'm realising maybe we should add a unit test for this case (Case 1) even though the Java implementation doesn't have one. We do have https://github.com/apache/iceberg-python/blob/main/tests/table/test_locations.py#L85-L86 but it doesn't test that entropies were injected for object storage. I've put up #1511.

Let me know if your thoughts regarding a potential WRITE_OBJECT_STORE_PARTITIONED_PATHS_DEFAULT change given this.

- Removed table proper

…test

kevinjqliu

LGTM! We'd probably want to document this but i see #1510 is opened to track

Fokko · 2025-01-13T14:53:13Z

Thanks for the quick follow up @smaheshwar-pltr, and thanks for the review @kevinjqliu 🙌

Make object storage the default location provider

30c77fc

smaheshwar-pltr commented Jan 11, 2025

View reviewed changes

smaheshwar-pltr mentioned this pull request Jan 11, 2025

Documentation for Location Providers #1510

Open

kevinjqliu reviewed Jan 11, 2025

View reviewed changes

Sreesh Maheshwar added 2 commits January 11, 2025 23:32

Nit: Remove comment beside property to prefer docs

2856e53

- Removed table proper

Nit: Add asserts for table properties defaults as well as comment in …

9980b39

…test

smaheshwar-pltr mentioned this pull request Jan 12, 2025

Improve LocationProvider unit tests #1511

Merged

kevinjqliu approved these changes Jan 12, 2025

View reviewed changes

kevinjqliu requested a review from Fokko January 12, 2025 19:12

Fokko approved these changes Jan 13, 2025

View reviewed changes

Fokko merged commit c409678 into apache:main Jan 13, 2025
7 checks passed

smaheshwar-pltr deleted the smaheshwar/object-store-default-provider branch January 13, 2025 15:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use `ObjectStoreLocationProvider` by default #1509

Use `ObjectStoreLocationProvider` by default #1509

smaheshwar-pltr commented Jan 11, 2025

smaheshwar-pltr Jan 11, 2025

kevinjqliu Jan 11, 2025

smaheshwar-pltr Jan 11, 2025 •

edited

Loading

smaheshwar-pltr commented Jan 11, 2025 •

edited

Loading

smaheshwar-pltr commented Jan 11, 2025 •

edited

Loading

kevinjqliu left a comment

kevinjqliu Jan 11, 2025

smaheshwar-pltr Jan 11, 2025 •

edited

Loading

kevinjqliu Jan 11, 2025

smaheshwar-pltr commented Jan 11, 2025 •

edited

Loading

kevinjqliu left a comment

Fokko commented Jan 13, 2025

Use ObjectStoreLocationProvider by default #1509

Use ObjectStoreLocationProvider by default #1509

Conversation

smaheshwar-pltr commented Jan 11, 2025

smaheshwar-pltr Jan 11, 2025

Choose a reason for hiding this comment

kevinjqliu Jan 11, 2025

Choose a reason for hiding this comment

smaheshwar-pltr Jan 11, 2025 • edited Loading

Choose a reason for hiding this comment

smaheshwar-pltr commented Jan 11, 2025 • edited Loading

smaheshwar-pltr commented Jan 11, 2025 • edited Loading

kevinjqliu left a comment

Choose a reason for hiding this comment

kevinjqliu Jan 11, 2025

Choose a reason for hiding this comment

smaheshwar-pltr Jan 11, 2025 • edited Loading

Choose a reason for hiding this comment

kevinjqliu Jan 11, 2025

Choose a reason for hiding this comment

smaheshwar-pltr commented Jan 11, 2025 • edited Loading

kevinjqliu left a comment

Choose a reason for hiding this comment

Fokko commented Jan 13, 2025

Use `ObjectStoreLocationProvider` by default #1509

Use `ObjectStoreLocationProvider` by default #1509

smaheshwar-pltr Jan 11, 2025 •

edited

Loading

smaheshwar-pltr commented Jan 11, 2025 •

edited

Loading

smaheshwar-pltr commented Jan 11, 2025 •

edited

Loading

smaheshwar-pltr Jan 11, 2025 •

edited

Loading

smaheshwar-pltr commented Jan 11, 2025 •

edited

Loading